Skip to content

Added support for passing tool_call_id via the RunContextWrapper #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

niv-hertz
Copy link

This PR fixes issue: #559

By adding the tool_call_id to the RunContextWrapper prior to calling tools. This gives the ability to access the tool_call_id in the implementation of the tool.

@niv-hertz
Copy link
Author

@rm-openai - This addressed the issue we discussed in another thread, would love you review :)

Copy link
Collaborator

@rm-openai rm-openai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delayed review. Was thinking about it, and it feels a bit bad to have tool_call_id be nullable.

What if instead we had ToolContext subclass RunContextWrapper. That way, existing usecases would just work since the downcast is fine. And tool_call_id would be non-null always?

@niv-hertz
Copy link
Author

niv-hertz commented Jun 2, 2025

Sorry for delayed review. Was thinking about it, and it feels a bit bad to have tool_call_id be nullable.

What if instead we had ToolContext subclass RunContextWrapper. That way, existing usecases would just work since the downcast is fine. And tool_call_id would be non-null always?

@rm-openai - I added a ToolContext object such that new users will be able to create tool functions that receive the ToolContext instead of the AgentContextWrapper. I also modified the internal FunctionTool class to only receive the ToolContext object in the on_invoke_tool params, so this change will break the type checking of anyone who uses the FunctionTool class directly instead of the wrappers. It seemed reasonable to me, let me know if you think otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants